Oh yeah, I also meant to test or ask about what happens when someone already set something outside of the guardrails before the guardrail was added. Will it just silently reset it within the guardrails the next time the preference menu is open? We should also at least an an issue ticket to the docs repo to add instructions to alter guardrails in the customization section of the docs

Maybe a popup/alert should happen whenever it’s reset? It could declare the enforced limits

And not sure if some actual guardrails should be included in this PR or wait for a future PR, but here are some suggestions:

Max Daily Safety Multiplier: 1 -
Current Basal Safety Multiplier: 1 -
Autosens Max: 1 - 3
Autosens Min: 0.1 - 1
AF: 0.1 - 3
Sigmoid AF: 0.1 - 2
Weight of past 24hr vs 10days: 0 - 1
Threshold: 60 - 120
Max Delta-BG Threshold: 0 - 0.4
Enable SMB When Glucose Is Over: 40 - 399
Max SMB Basal Mins: 0 - 180
Max UAM SMB Basal Mins: 0 - 180
SMB Delivery Ratio: 0 - 1
Bolus Increment: 0.025 -
Autotune ISF AF: 0 - 1

Oh yeah, I also meant to test or ask about what happens when someone already set something outside of the guardrails before the guardrail was added. Will it just silently reset it within the guardrails the next time the preference menu is open? We should also at least an an issue ticket to the docs repo to add instructions to alter guardrails in the customization section of the docs

Maybe a popup/alert should happen whenever it’s reset? It could declare the enforced limits

I think it's is incredibly important to define. IMO throwing a pop-up telling people that this is changed (on save), and then we should have a discussion on what should happen after that. I can see the following ways of handling it:

1. Clamp to whatever end of the guardrail that is closer
2. Require the user to set it themselves
3. Start warning on settings save in one version, but not clamp or require any user input changes. Then in a later version we start doing either 1 or 2.

I am not personally sure which approach is better.

The 90 g min limit did clamp values below to 90. There was no max value, so I don’t know if the same happens when above (clamping to the closest limit?)

I can confirm that the current implementation clamps the setting to the closest limit, after testing to introduction of an arbitrary max limit as well:

                Field(
                    displayName: NSLocalizedString("Remaining Carbs Cap", comment: "Remaining Carbs Cap"),
                    type: .decimal(keypath: \.remainingCarbsCap, minVal: 90, maxVal: 100),
                    infoText: NSLocalizedString(
                        "This is the amount of the maximum number of carbs we’ll assume will absorb over 4h if we don’t yet see carb absorption.",
                        comment: "Remaining Carbs Cap"
                    ),

< min limit = min limit
> max limit = max limit

(settings in between are accepted as is)

Just tested this on some max/min for decimal prefs. Works as suggested and if going beyond min/max it will restricted to applicable min/max. On Bool prefs it throws an error while configuring it. So all good!!v

I suggest to define the min/max limits in FreeAPS/Sources/Models/Preferences.swift though, instead of in the FreeAPS/Sources/Modules/PreferencesEditor/PreferencesEditorStateModel.swift

I suggest to define the min/max limits in FreeAPS/Sources/Models/Preferences.swift though, instead of in the FreeAPS/Sources/Modules/PreferencesEditor/PreferencesEditorStateModel.swift

That might be a good idea for maintainability, then defaults and min/max limits could be stored the same place?

So here:
https://github.com/nightscout/Trio/blob/dev/FreeAPS/Sources/Models/Preferences.swift

Instead of here:

@JeremyStorring , thoughts?

I suggest to define the min/max limits in FreeAPS/Sources/Models/Preferences.swift though, instead of in the FreeAPS/Sources/Modules/PreferencesEditor/PreferencesEditorStateModel.swift

That might be a good idea for maintainability, then defaults and min/max limits could be stored the same place?

So here: https://github.com/nightscout/Trio/blob/dev/FreeAPS/Sources/Models/Preferences.swift

Instead of here:

Trio/FreeAPS/Sources/Modules/PreferencesEditor/PreferencesEditorStateModel.swift

Line 448 in 1941a02

type: .decimal(keypath: \.remainingCarbsCap, minVal: 90),

@JeremyStorring , thoughts?

I can definitely give this a shot, but it might be a bit harder to implement :)

@JeremyStorring

I tested the PR with success.

I suggest to define the min/max limits in FreeAPS/Sources/Models/Preferences.swift though, instead of in the FreeAPS/Sources/Modules/PreferencesEditor/PreferencesEditorStateModel.swift
I can definitely give this a shot, but it might be a bit harder to implement :)

You could use the same approach as the key path with this suggestion (I quickly tested) :

case decimal(
            keypath: WritableKeyPath<Preferences, Decimal>,
            minVal: WritableKeyPath<Preferences, Decimal>? = nil,
            maxVal: WritableKeyPath<Preferences, Decimal>? = nil
        )
....
if let minValue = minVal, let minValueDecimal: Decimal = settable?.get(minValue),
                   let maxValue = maxVal, let maxValueDecimal: Decimal = settable?.get(maxValue)
                {
                    constrainedValue = min(max(value, minValueDecimal), maxValueDecimal)
                } else if let minValue = minVal, let minValueDecimal: Decimal = settable?.get(minValue) {
                    constrainedValue = max(value, minValueDecimal)
                } else if let maxValue = maxVal, let maxValueDecimal: Decimal = settable?.get(maxValue) {
                    constrainedValue = min(value, maxValueDecimal)
                } else {
                    constrainedValue = value
                }

and after just to write each min / max with :

Field(
                    displayName: NSLocalizedString("Remaining Carbs Cap", comment: "Remaining Carbs Cap"),
                    type: .decimal(
                        keypath: \.remainingCarbsCap,
                        minVal: \.remainingCarbsCap_min,
                        maxVal: \.remainingCarbsCap_max
                    ),
                    ....
                ),

and specify in preferences.swift :

 var remainingCarbsCap_min: Decimal = 80
    var remainingCarbsCap_max: Decimal = 100

minVal: WritableKeyPath<Preferences, Decimal>? = nil,
maxVal: WritableKeyPath<Preferences, Decimal>? = nil

Oh boy I completely misunderstood what was being asked, yeah this makes sense and I think would be a better implementation!

I didn't actually add min and max values, but it works as intended once we updated Preferences.

Also disregard git fail from above :P

Thanks!

Could you please add min/max limits for one or more of the settings that are likely to be agreement on, as an example to ease the code review and testing?

Such as:

Weight of past 24hr vs 10days: 0 - 1
Threshold: 60 - 120
SMB Delivery Ratio: 0 - 1
Autotune ISF AF: 0 - 1

As. this only adds functionality to limit preferences and does not actually introduce any limits (for now) LGTM.

Merging with 2 approvals and mine😇